Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix removal of storage variable with fatdb. #38

Merged
merged 3 commits into from
Sep 5, 2018

Conversation

cheme
Copy link
Collaborator

@cheme cheme commented Aug 23, 2018

On openethereum/parity-ethereum#9180 use of fatdb with an archive node leads to this situation :
in a block :

  • tr1 sstore 0 at address a delete is send to archivedb through fatdb
  • tr2 sstore a val at same address an insert to archivedb through fatdb (the bug is that it is not the same address because delete do not use the right storing address)
  • tr3 sstore 0 again and a second delete is send to archive

(double delete at a address due to the intermediate insert done at another address -> break assertion of reference count max at 1 one an archive node).
So this fix fatdb to delete and insert at same address (I kept the scheme of double hashing because it is consistent with the fatdb query and existing test cases but I am not sure it is needed, if a reviewer can point out the need for this double hashing it interests me).

@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@dvdplm
Copy link
Contributor

dvdplm commented Aug 23, 2018

@rphmeier explained the double hashing but I've already forgotten what it was. :/ It is indeed needed though.

@dvdplm dvdplm requested a review from rphmeier August 23, 2018 16:07
@cheme
Copy link
Collaborator Author

cheme commented Aug 23, 2018

My first guess would be to get an uniform address space distribution (96 bits of hashdb key are not).

@cheme
Copy link
Collaborator Author

cheme commented Aug 24, 2018

There is something tricky with this removal : in our use case (archivedb) we do not want it to actually happen except at a single block level (so we can query at previous block) and it will not due to archivedb implementation.
But if we run fatdb in non archive mode, this deletion may not be what we want and we may prefer to remove those deletion lines (we will get additional false value in the db but it is only an issue for the disk space).
I am really not sure if using the fatdb without archive makes sense (parity_listStorageKeys seems to be the only method that is impacted by this suppression).
If it makes sense (non archive + fatdb), an alternate fix would be to avoid removing this association at all.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about this code but the changes LGTM. Regarding your questions of using fatdb with non-archive pruning, the fatdb should only be able to enumerate keys that are present right? So it's fine to remove the association regardless of the pruning mode?

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cheme could you add a test which inserts and then removes a key?

@dvdplm
Copy link
Contributor

dvdplm commented Sep 5, 2018

@cheme I agree that the current behaviour looks broken. I don't fully understand what the purpose of FatDB is but this test fails on master and passes on this branch (@ordian is this sort of what you were asking for?):

	#[test]
	fn insert_and_removal_leaves_the_db_empty() {
		use hashdb::HashDB;
		let mut db = MemoryDB::<KeccakHasher>::new();
		let mut root = H256::new();
		{
			let mut t = FatDBMut::new(&mut db, &mut root);
			t.insert(&[1], &[2]).unwrap();
			t.remove(&[1]).unwrap();
		}
		assert_eq!(db.keys().len(), 0);
	}

@ordian
Copy link
Member

ordian commented Sep 5, 2018

@dvdplm yes, a similar test was added already by @cheme.

@dvdplm dvdplm merged commit bbe809d into paritytech:master Sep 5, 2018
@ordian
Copy link
Member

ordian commented Sep 5, 2018

@dvdplm should we publish patricia-tree 0.2.2 and update parity-ethereum and substrate as well?

@andresilva
Copy link
Contributor

Please reference openethereum/parity-ethereum#9213 when updating the dependency on parity-ethereum so that the original issue is closed. 👍

@dvdplm
Copy link
Contributor

dvdplm commented Sep 6, 2018

should we publish patricia-tree 0.2.2

Yes. I have pushed out 0.2.2-beta.0. When we feel confident that openethereum/parity-ethereum#9213 is fixed we'll get it off the beta. In-progress PR here.

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants